-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a shared extractor library #12546
Conversation
efc8b34
to
3829454
Compare
This makes it possible for different languages to share this extractor.
This mirrors the structure we have in the Ruby extractor, and will allow us to share more code.
Ensure that builds via cargo-cross, which are executed in a docker container, can see the shared library.
3829454
to
856132b
Compare
This include support for mounting external path dependencies as volumes.
0fc65dc
to
24baf64
Compare
24baf64
to
30eacd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. A couple of suggestions.
.github/workflows/ruby-build.yml
Outdated
@@ -60,8 +60,10 @@ jobs: | |||
path: | | |||
ruby/extractor/target/release/autobuilder | |||
ruby/extractor/target/release/autobuilder.exe | |||
ruby/extractor/target/x86_64-unknown-linux-gnu/release/autobuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? It looks like you are mv
-ing those paths when building the extractor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. I've removed these paths.
run: cd extractor && cross build --release | ||
run: | | ||
cd extractor | ||
cross build --release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use --target-dir
instead of using mv
afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that will work. The default target-dir
is ./target
. Inside this dir, cross creates the x86_64-unknown-linux-gnu
directory.
shared/extractor/src/options.rs
Outdated
/// assert_eq!(parse_codeql_threads("-5", 4), Some(1)); | ||
/// assert_eq!(parse_codeql_threads("nope", 4), None); | ||
/// ``` | ||
pub fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> { | |
fn parse_codeql_threads(threads_str: &str, num_cpus: usize) -> Option<usize> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we need the pub
for the doc tests to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just annoying. I quite like the doc tests. Let's leave it.
@@ -14,13 +14,21 @@ else | |||
fi | |||
|
|||
(cd extractor && "$CARGO" build --release) | |||
extractor/target/release/generator --dbscheme ql/lib/ruby.dbscheme --library ql/lib/codeql/ruby/ast/internal/TreeSitter.qll | |||
|
|||
# If building via cross, the binaries will be in extractor/target/<triple>/release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we try to use the --target-dir
flag to make cross
output the files to the same directory as cargo
would?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - I'll give that a try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this doesn't work - see my other comment for more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rather put it in shared/tree-sitter-extractor
, to make it clear that is for TreeSitter based extractors?
It is now called `tree-sitter-extractor`, to make it clearer that it builds on tree-sitter grammars.
Move most of the common code between the Ruby and QL extractors into a library at
shared/extractor
.